-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Added a local digest command to the client to execute the XXH3 locally… #3884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
… rather than asking the server for it
… as an optional dependency
…for cosmetical reasons
…n of decode_responces setting. Adding test that validates the digets_local return type as well as the returned value.
5c02c59 to
bffee09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds a new digest_local command to compute XXH3 hashes locally on the client side, eliminating the need for a server roundtrip. This is particularly useful for conditional operations like IFDEQ/IFDNE where the digest needs to be computed client-side before sending commands.
Changes:
- Added
digest_localmethod to compute XXH3 digests locally using the xxhash library - Added xxhash as an optional dependency with version constraint ~=3.6.0
- Added comprehensive tests for both sync and async clients to verify local digest matches server digest
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| redis/commands/core.py | Implements the digest_local method with xxhash library import handling and encoding/decoding logic |
| pyproject.toml | Adds xxhash as an optional dependency under the [project.optional-dependencies] section |
| dev_requirements.txt | Pins xxhash==3.6.0 for development environment |
| tests/test_commands.py | Adds test to verify local digest matches server digest, and integrates digest_local into existing IFDEQ/IFDNE tests |
| tests/test_asyncio/test_commands.py | Adds async version of the local digest test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local_digest = xxhash.xxh3_64(value).hexdigest() | ||
|
|
||
| # To align with digest, we want to return bytes if decode_responses is False. | ||
| # The following should work because Python's mixin approach. |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "The following should work because Python's mixin approach" is unclear and grammatically incomplete. Consider revising to: "The following should work because of Python's mixin approach" or providing more context about what specifically about the mixin approach makes this work.
| # The following should work because Python's mixin approach. | |
| # The following works because of Python's mixin-based client class hierarchy. |
| if isinstance(res_server, bytes): | ||
| assert isinstance(res_local, bytes) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies that res_local is bytes when res_server is bytes, but doesn't verify the opposite case when decode_responses=True. Consider adding an else clause to assert that both are strings when decode_responses=True, ensuring type consistency between server and local digest in both modes.
| @pytest.mark.parametrize( | ||
| "value", [b"", b"abc", b"The quick brown fox jumps over the lazy dog"] | ||
| ) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only covers bytes values but the digest_local method accepts Union[bytes, str] according to its type annotation. Consider adding test cases with string values to ensure the method correctly handles both bytes and str inputs, especially considering encoding issues.
| @pytest.mark.parametrize( | ||
| "value", [b"", b"abc", b"The quick brown fox jumps over the lazy dog"] | ||
| ) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only covers bytes values but the digest_local method accepts Union[bytes, str] according to its type annotation. Consider adding test cases with string values to ensure the method correctly handles both bytes and str inputs, especially considering encoding issues.
|
|
||
| # Try to import the xxhash library as an optional dependency | ||
| try: | ||
| import xxhash | ||
|
|
||
| HAS_XXHASH = True | ||
| except ImportError: | ||
| HAS_XXHASH = False | ||
|
|
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optional import block should be placed after other standard library imports to follow PEP 8 import ordering conventions. The blank line on line 5 creates unnecessary separation in the standard library imports section. Consider moving the try-except block to after line 14 (after the warnings import) and removing the blank line at line 5.
| def digest_local(self, value: Union[bytes, str]) -> Union[bytes, str]: | ||
| """ | ||
| Compute the hexadecimal digest of the value locally, without sending it to the server. | ||
| This is useful for conditional operations like IFDEQ/IFDNE where you need to | ||
| compute the digest client-side before sending a command. | ||
| Warning: | ||
| **Experimental** - This API may change or be removed without notice. | ||
| Arguments: | ||
| - value: Union[bytes, str] - the value to compute the digest of. | ||
| Returns: | ||
| - (str | bytes) the XXH3 digest of the value as a hex string (16 hex characters) | ||
| For more information, see https://redis.io/commands/digest | ||
| """ | ||
| if not HAS_XXHASH: | ||
| raise NotImplementedError( | ||
| "XXHASH library not installed. Install with: " | ||
| "'pip install xxhash' or 'pip install redis[xxhash]' to use this feature." | ||
| ) | ||
|
|
||
| local_digest = xxhash.xxh3_64(value).hexdigest() | ||
|
|
||
| # To align with digest, we want to return bytes if decode_responses is False. | ||
| # The following should work because Python's mixin approach. | ||
| if not self.get_encoder().decode_responses: | ||
| local_digest = local_digest.encode() | ||
|
|
||
| return local_digest |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for the case when the xxhash library is not installed. Consider adding a test that verifies the NotImplementedError is raised with an appropriate message when digest_local is called without the xxhash dependency available. This would ensure the error handling path works correctly.
| if isinstance(res_server, bytes): | ||
| assert isinstance(res_local, bytes) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies that res_local is bytes when res_server is bytes, but doesn't verify the opposite case when decode_responses=True. Consider adding an else clause to assert that both are strings when decode_responses=True, ensuring type consistency between server and local digest in both modes.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Added a local digest command to the client to execute the XXH3 locally rather than using a server roundtrip.
@petyaslavova I added this for now as a command next to the
digestcommand. There are indeed some discussion points:digest_local?digestcommand instead to have a booleanexec_localparameter?BTW: This adds
xxhash~=3.6.0as an additional dependency.